Keycloak modules retry request on authentication error, support refresh token parameter #9494

armkeh · 2024-12-30T23:36:06Z

SUMMARY

Wraps all requests to Keycloak in the Keycloak modules (keycloak_authentication, keycloak_authz_authorization_scope, keycloak_authz_custom_policy, etc.) with retry logic to make use of a new refresh_token module parameter.

This improves the user experience when using Keycloak modules with the auth_token parameter; previously if that token expired during playbook execution, subsequent tasks would fail. Now they "fall back" to using the refresh_token, or, if it is not provided or is expired itself, to using the auth_username and auth_password.

    def _request(self, url, method, data=None):
        def make_request_ignoring_401():
            try:
                return open_url(url, method=method, data=data,
                                http_agent=self.http_agent, headers=self.restheaders,
                                timeout=self.connection_timeout,
                                validate_certs=self.validate_certs)
            except HTTPError as e:
                if e.code != 401:
                    raise e

            return None

        r = make_request_ignoring_401()
        if r is not None:
            return r

        # Authentication may have expired, re-authenticate with refresh token and retry
        refresh_token = self.module.params.get('refresh_token')
        if refresh_token is not None:
            token = _get_token_using_refresh_token(self.module.params)
            self.restheaders['Authorization'] = 'Bearer ' + token

        r = make_request_ignoring_401()
        if r is not None:
            return r

        # Retry once more with username and password
        auth_username = self.module.params.get('auth_username')
        auth_password = self.module.params.get('auth_password')
        if auth_username is not None and auth_password is not None:
            token = _get_token_using_credentials(self.module.params)
            self.restheaders['Authorization'] = 'Bearer ' + token

        return make_request_ignoring_401()

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

keycloak

…not yet working [8857]

…eeded [8857]

…ith retry logic [8857]

…l [8857]

… [8857]

… credentials [8857]

…[8857]

…n, valid refresh token [8857]

…without username/pass [8857]

…857]

russoz

hi @armkeh thanks for your contribution!

Couple of comments on the PR.

plugins/modules/keycloak_authentication.py

plugins/module_utils/identity/keycloak/keycloak.py

…assword params [8857]

plugins/modules/keycloak_authentication.py

plugins/doc_fragments/keycloak.py

russoz

LGTM

felixfontein

Thanks for your contribution! Please add a changelog fragment. Thanks.

plugins/module_utils/identity/keycloak/keycloak.py

…patability [8857]

plugins/module_utils/identity/keycloak/keycloak.py

…h token not provided (ansible-collections#8857)

…rn exception on failure (ansible-collections#8857)

russoz

Couple of comments more.

russoz · 2025-01-23T02:11:49Z

plugins/module_utils/identity/keycloak/keycloak.py

+    def _request(self, url, method, data=None):
+        def make_request_catching_401():
+            try:
+                return open_url(url, method=method, data=data,
+                                http_agent=self.http_agent, headers=self.restheaders,
+                                timeout=self.connection_timeout,
+                                validate_certs=self.validate_certs)
+            except HTTPError as e:
+                if e.code != 401:
+                    raise e
+                return e
+
+        r = make_request_catching_401()
+        if not isinstance(r, Exception):
+            return r
+


Little late in the game for that, I know, but I believe this would be leaner as:

Suggested change

def _request(self, url, method, data=None):

def make_request_catching_401():

try:

return open_url(url, method=method, data=data,

http_agent=self.http_agent, headers=self.restheaders,

timeout=self.connection_timeout,

validate_certs=self.validate_certs)

except HTTPError as e:

if e.code != 401:

raise e

return e

r = make_request_catching_401()

if not isinstance(r, Exception):

return r

def _request(self, url, method, data=None):

class Unauthorized(Exception):

pass

def make_request_catching_401():

try:

return open_url(url, method=method, data=data,

http_agent=self.http_agent, headers=self.restheaders,

timeout=self.connection_timeout,

validate_certs=self.validate_certs)

except HTTPError as e:

if e.code != 401:

raise e

raise Unauthorized()

try:

return make_request_catching_401()

exception Unauthorized:

pass

except Exception:

raise

and so on

I like this, however we need to keep the original 401 exception in case no re-auth options are available and it needs to be re-raised. (Thanks for catching the mistake where it was being returned and not raised in your other comment.)

russoz · 2025-01-23T02:26:54Z

plugins/module_utils/identity/keycloak/keycloak.py

+            r = make_request_catching_401()
+
+        return r
+


This last call to make_request_catching_401() is not being verified. If, for whatever reason, the HTTP request inside that call returns a 401, that Exception object will be returned (not raised) and no error handling will be performed.

Thanks, good catch; I've reworked the retry wrappers to make the flow a little clearer, removing early returns, and made sure to throw the exception if that's the final result.

…ions are thrown (ansible-collections#8857)

…ble-collections#8857)

…ble-collections#8857) Add test to verify this behaviour works.

…thentication failures (ansible-collections#8857)

…ay (ansible-collections#8857)

russoz

one possible improvement and a comment, other than that LGTM

russoz · 2025-01-23T05:35:26Z

plugins/module_utils/identity/keycloak/keycloak.py

+    try:
+        r = json.loads(to_native(open_url(auth_url, method='POST',
+                                          validate_certs=validate_certs, http_agent=http_agent, timeout=connection_timeout,
+                                          data=urlencode(payload)).read()))
+    except ValueError as e:
+        raise KeycloakError(
+            'API returned invalid JSON when trying to obtain access token from %s: %s'
+            % (auth_url, str(e)))
+    except Exception as e:
+        raise KeycloakError('Could not obtain access token from %s: %s'
+                            % (auth_url, str(e)), authError=e)
+
+    try:
+        token = r['access_token']
+    except KeyError:
+        raise KeycloakError(
+            'Could not obtain access token from %s' % auth_url)
+
+    return token


It might be simpler to go:

Suggested change

try:

r = json.loads(to_native(open_url(auth_url, method='POST',

validate_certs=validate_certs, http_agent=http_agent, timeout=connection_timeout,

data=urlencode(payload)).read()))

except ValueError as e:

raise KeycloakError(

'API returned invalid JSON when trying to obtain access token from %s: %s'

% (auth_url, str(e)))

except Exception as e:

raise KeycloakError('Could not obtain access token from %s: %s'

% (auth_url, str(e)), authError=e)

try:

token = r['access_token']

except KeyError:

raise KeycloakError(

'Could not obtain access token from %s' % auth_url)

return token

try:

r = json.loads(to_native(open_url(auth_url, method='POST',

validate_certs=validate_certs, http_agent=http_agent, timeout=connection_timeout,

data=urlencode(payload)).read()))

return r['access_token']

except ValueError as e:

raise KeycloakError(

'API returned invalid JSON when trying to obtain access token from %s: %s'

% (auth_url, str(e)))

except KeyError:

raise KeycloakError(

'Could not obtain access token from %s' % auth_url)

except Exception as e:

raise KeycloakError('Could not obtain access token from %s: %s'

% (auth_url, str(e)), authError=e)

or remove the except KeyError block entirely - the raised error after that is almost the same.

Done. Rather than exclude the KeyError catch, I made it's message clearer about the problem (response did not include access_token).

russoz · 2025-01-23T08:06:27Z

plugins/module_utils/identity/keycloak/keycloak.py

+            except HTTPError as e:
+                if e.code != 401:
+                    raise e
+                return e


This still bugs me on principle - exceptions are meant to be raised not returned :-) - but let's not hold this PR back because of it. I'll make a note to myself to return to this later and see if I can make it any better.

…ctions#8857)

felixfontein · 2025-01-25T09:32:25Z

LGTM as well.

…name/password (ansible-collections#8857) To emphasize their difference from the `get_token` function, which either gets the token from the module params *or* makes a request for it.

…ble-collections#8857)

… error during auth request (ansible-collections#8857)

armkeh · 2025-01-26T03:08:45Z

Thank you all for the feedback! I've addressed the comments (except @russoz's regarding the helper function returning an exception, for which a neat refactor isn't obvious), and added docstrings to the functions I've added or significantly reworked; so outside of any other feedback, I think this is ready to go.

One note, there are some integration tests failing for the Keycloak modules; I confirmed those same tests fail on the main branch currently. I'm planning to try and repair them in a separate PR.

patchback · 2025-01-26T14:23:42Z

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/af0118278b5f8ce2f587ee2f5121f43de8ba4b6b/pr-9494

Backported as #9631

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

…sh token parameter (#9494) * feat: begin refactor to support refresh token in keycloak modules * chore: add start of tests for shared token usage * feat: progress towards supporting refresh token; token introspection not yet working [8857] * chore: reset to main branch previous state; a different approach is needed [8857] * feat: add request methods to keycloak class, which will be expanded with retry logic [8857] * feat: all requests to keycloak use request methods instead of open_url [8857] * fix: data argument is optional in keycloak request methods [8857] * feat: add integration test for keycloak module authentication methods [8857] * chore: refactor get token logic to separate logic using username/pass credentials [8857] * chore: refactor token request logic further to isolate request logic [8857] * chore: fix minor lint issues [8857] * test: add (currently failing) test for request with invalid auth token, valid refresh token [8857] * chore: allow realm to be provided to role module with refresh_token, without username/pass [8857] * feat: add retry logic to requests in keycloak module utils [8857] * chore: rename keycloak module fail_open_url method to fail_request [8857] * chore: update all keycloak modules to support refresh token param [8857] * chore: add refresh_token param to keycloak doc_fragments [8857] * chore: restore dependency between auth_realm and auth_username,auth_password params [8857] * chore: rearrange module param checks to reduce future pr size [8857] * chore: remove extra comma [8857] * chore: update version added for refresh token param [8857] * chore: add changelog fragment [8857] * chore: re-add fail_open_url to keycloak module utils for backward compatability [8857] * fix: do not make a new request to keycloak without reauth when refresh token not provided (#8857) * fix: only make final auth attempt if username/pass provided, and return exception on failure (#8857) * fix: make re-auth and retry code more consistent, ensure final exceptions are thrown (#8857) * test: fix arguments for invalid token, valid refresh token test (#8857) * feat: catch invalid refresh token errors during re-auth attempt (#8857) Add test to verify this behaviour works. * test: improve test coverage, including some unhappy path tests for authentication failures (#8857) * chore: store auth errors from token request in backwards compatible way (#8857) * fix: ensure method is still specified for all requests (#8857) * chore: simplify token request logic (#8857) * chore: rename functions to request tokens using refresh token or username/password (#8857) To emphasize their difference from the `get_token` function, which either gets the token from the module params *or* makes a request for it. * doc: add docstrings for new or significantly modified functions (#8857) * test: repair unit test following change to exception message upon key error during auth request (#8857) (cherry picked from commit af01182)

felixfontein · 2025-01-26T14:24:01Z

@armkeh thanks for your contribution!
@desand01 @russoz thanks for reviewing!

…t on authentication error, support refresh token parameter (#9631) Keycloak modules retry request on authentication error, support refresh token parameter (#9494) * feat: begin refactor to support refresh token in keycloak modules * chore: add start of tests for shared token usage * feat: progress towards supporting refresh token; token introspection not yet working [8857] * chore: reset to main branch previous state; a different approach is needed [8857] * feat: add request methods to keycloak class, which will be expanded with retry logic [8857] * feat: all requests to keycloak use request methods instead of open_url [8857] * fix: data argument is optional in keycloak request methods [8857] * feat: add integration test for keycloak module authentication methods [8857] * chore: refactor get token logic to separate logic using username/pass credentials [8857] * chore: refactor token request logic further to isolate request logic [8857] * chore: fix minor lint issues [8857] * test: add (currently failing) test for request with invalid auth token, valid refresh token [8857] * chore: allow realm to be provided to role module with refresh_token, without username/pass [8857] * feat: add retry logic to requests in keycloak module utils [8857] * chore: rename keycloak module fail_open_url method to fail_request [8857] * chore: update all keycloak modules to support refresh token param [8857] * chore: add refresh_token param to keycloak doc_fragments [8857] * chore: restore dependency between auth_realm and auth_username,auth_password params [8857] * chore: rearrange module param checks to reduce future pr size [8857] * chore: remove extra comma [8857] * chore: update version added for refresh token param [8857] * chore: add changelog fragment [8857] * chore: re-add fail_open_url to keycloak module utils for backward compatability [8857] * fix: do not make a new request to keycloak without reauth when refresh token not provided (#8857) * fix: only make final auth attempt if username/pass provided, and return exception on failure (#8857) * fix: make re-auth and retry code more consistent, ensure final exceptions are thrown (#8857) * test: fix arguments for invalid token, valid refresh token test (#8857) * feat: catch invalid refresh token errors during re-auth attempt (#8857) Add test to verify this behaviour works. * test: improve test coverage, including some unhappy path tests for authentication failures (#8857) * chore: store auth errors from token request in backwards compatible way (#8857) * fix: ensure method is still specified for all requests (#8857) * chore: simplify token request logic (#8857) * chore: rename functions to request tokens using refresh token or username/password (#8857) To emphasize their difference from the `get_token` function, which either gets the token from the module params *or* makes a request for it. * doc: add docstrings for new or significantly modified functions (#8857) * test: repair unit test following change to exception message upon key error during auth request (#8857) (cherry picked from commit af01182) Co-authored-by: Mark Armstrong <[email protected]>

11.2.0 Major Changes community.general - keycloak_* modules - ``refresh_token`` parameter added. When multiple authentication parameters are provided (``token``, ``refresh_token``, and ``auth_username``/``auth_password``), modules will now automatically retry requests upon authentication errors (401), using in order the token, refresh token, and username/password (ansible-collections/community.general#9494). community.vmware - vmware_dvswitch_pvlans - The VLAN ID type has been updated to be handled as an integer (ansible-collections/community.vmware#2267). dellemc.openmanage - omevv_firmware - This module allows to update firmware of the single host and single cluster. fortinet.fortios - Support check_mode on all the configuration modules. google.cloud - google_cloud_ops_agents - role submodule removed because it prevents the collection from passing sanity and lint tests grafana.grafana - Ability to set custom directory path for *.alloy config files - Fix 'dict object' has no attribute 'path' when running with --check - Update grafana template - add loki bloom support - grafana.ini yaml syntax

armkeh added 21 commits September 12, 2024 23:38

feat: begin refactor to support refresh token in keycloak modules

31e3791

chore: add start of tests for shared token usage

3fe444a

feat: progress towards supporting refresh token; token introspection …

c7341db

…not yet working [8857]

chore: reset to main branch previous state; a different approach is n…

670db1a

…eeded [8857]

Merge remote-tracking branch 'upstream/main'

016f5fe

feat: add request methods to keycloak class, which will be expanded w…

b7fd1e8

…ith retry logic [8857]

chore: Merge remote-tracking branch 'upstream/main' [8857]

70813f3

feat: all requests to keycloak use request methods instead of open_ur…

b73c82b

…l [8857]

chore: Merge remote-tracking branch 'upstream/main' [8857]

bfe339a

fix: data argument is optional in keycloak request methods [8857]

d6a2730

feat: add integration test for keycloak module authentication methods…

8c7684a

… [8857]

chore: refactor get token logic to separate logic using username/pass…

08650d0

… credentials [8857]

chore: refactor token request logic further to isolate request logic …

ccea7aa

…[8857]

chore: fix minor lint issues [8857]

bd31be4

test: add (currently failing) test for request with invalid auth toke…

817c085

…n, valid refresh token [8857]

chore: allow realm to be provided to role module with refresh_token, …

6770085

…without username/pass [8857]

feat: add retry logic to requests in keycloak module utils [8857]

3b90af5

chore: rename keycloak module fail_open_url method to fail_request [8…

76a95ed

…857]

chore: merge remote-tracking branch 'upstream/main' [8857]

53eec38

chore: update all keycloak modules to support refresh token param [8857]

5b23f10

chore: merge remote-tracking branch 'upstream/main' [8857]

a782e11

russoz reviewed Dec 31, 2024

View reviewed changes

plugins/modules/keycloak_authentication.py Outdated Show resolved Hide resolved

plugins/module_utils/identity/keycloak/keycloak.py Show resolved Hide resolved

armkeh added 2 commits December 30, 2024 21:07

chore: add refresh_token param to keycloak doc_fragments [8857]

a58ea2b

chore: restore dependency between auth_realm and auth_username,auth_p…

85296f9

…assword params [8857]

russoz reviewed Dec 31, 2024

View reviewed changes

plugins/modules/keycloak_authentication.py Outdated Show resolved Hide resolved

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Dec 31, 2024

armkeh added 2 commits December 31, 2024 16:33

chore: rearrange module param checks to reduce future pr size [8857]

42dfe5d

chore: remove extra comma [8857]

c7a9207

russoz reviewed Jan 1, 2025

View reviewed changes

plugins/doc_fragments/keycloak.py Outdated Show resolved Hide resolved

russoz approved these changes Jan 1, 2025

View reviewed changes

felixfontein reviewed Jan 3, 2025

View reviewed changes

plugins/module_utils/identity/keycloak/keycloak.py Show resolved Hide resolved

plugins/module_utils/identity/keycloak/keycloak.py Outdated Show resolved Hide resolved

armkeh added 3 commits January 3, 2025 15:39

chore: add changelog fragment [8857]

24dbd85

chore: merge remote-tracking branch 'upstream/main' [8857]

f183ab5

chore: re-add fail_open_url to keycloak module utils for backward com…

3ebc757

…patability [8857]

desand01 reviewed Jan 18, 2025

View reviewed changes

plugins/module_utils/identity/keycloak/keycloak.py Outdated Show resolved Hide resolved

armkeh added 2 commits January 22, 2025 20:07

fix: do not make a new request to keycloak without reauth when refres…

ef79f26

…h token not provided (ansible-collections#8857)

fix: only make final auth attempt if username/pass provided, and retu…

ab26910

…rn exception on failure (ansible-collections#8857)

russoz reviewed Jan 23, 2025

View reviewed changes

armkeh added 5 commits January 22, 2025 21:47

fix: make re-auth and retry code more consistent, ensure final except…

e62e04b

…ions are thrown (ansible-collections#8857)

test: fix arguments for invalid token, valid refresh token test (ansi…

f37d7cf

…ble-collections#8857)

feat: catch invalid refresh token errors during re-auth attempt (ansi…

57270fb

…ble-collections#8857) Add test to verify this behaviour works.

test: improve test coverage, including some unhappy path tests for au…

a10f37b

…thentication failures (ansible-collections#8857)

chore: store auth errors from token request in backwards compatible w…

b8b79b1

…ay (ansible-collections#8857)

russoz reviewed Jan 23, 2025

View reviewed changes

armkeh added 2 commits January 25, 2025 03:58

fix: ensure method is still specified for all requests (ansible-colle…

3b4e3d4

…ctions#8857)

chore: simplify token request logic (ansible-collections#8857)

b3614f0

armkeh added 3 commits January 25, 2025 20:37

chore: rename functions to request tokens using refresh token or user…

ed0c90b

…name/password (ansible-collections#8857) To emphasize their difference from the `get_token` function, which either gets the token from the module params *or* makes a request for it.

doc: add docstrings for new or significantly modified functions (ansi…

70f7217

…ble-collections#8857)

test: repair unit test following change to exception message upon key…

e5da3ff

… error during auth request (ansible-collections#8857)

felixfontein merged commit af01182 into ansible-collections:main Jan 26, 2025
138 checks passed

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jan 26, 2025

patchback bot mentioned this pull request Jan 26, 2025

[PR #9494/af011827 backport][stable-10] Keycloak modules retry request on authentication error, support refresh token parameter #9631

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keycloak modules retry request on authentication error, support refresh token parameter #9494

Keycloak modules retry request on authentication error, support refresh token parameter #9494

armkeh commented Dec 30, 2024 •

edited

Loading

russoz left a comment

russoz left a comment

felixfontein left a comment

russoz left a comment

russoz Jan 23, 2025

armkeh Jan 23, 2025

russoz Jan 23, 2025

armkeh Jan 23, 2025

russoz left a comment

russoz Jan 23, 2025

armkeh Jan 26, 2025

russoz Jan 23, 2025

felixfontein commented Jan 25, 2025

armkeh commented Jan 26, 2025

patchback bot commented Jan 26, 2025 •

edited

Loading

felixfontein commented Jan 26, 2025

Keycloak modules retry request on authentication error, support refresh token parameter #9494

Keycloak modules retry request on authentication error, support refresh token parameter #9494

Conversation

armkeh commented Dec 30, 2024 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

russoz left a comment

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

felixfontein left a comment

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

russoz Jan 23, 2025

Choose a reason for hiding this comment

armkeh Jan 23, 2025

Choose a reason for hiding this comment

russoz Jan 23, 2025

Choose a reason for hiding this comment

armkeh Jan 23, 2025

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

russoz Jan 23, 2025

Choose a reason for hiding this comment

armkeh Jan 26, 2025

Choose a reason for hiding this comment

russoz Jan 23, 2025

Choose a reason for hiding this comment

felixfontein commented Jan 25, 2025

armkeh commented Jan 26, 2025

patchback bot commented Jan 26, 2025 • edited Loading

Backport to stable-10: 💚 backport PR created

felixfontein commented Jan 26, 2025

armkeh commented Dec 30, 2024 •

edited

Loading

patchback bot commented Jan 26, 2025 •

edited

Loading